sequenceDiagram
  participant Client as Client
  participant Server as SQSServer
  participant Catalog as QueueMetaStore
  participant Bucket as BucketStore
  participant Handler as QueueHandler

  Client->>Server: SendMessage / ReceiveMessage
  Server->>Catalog: Resolve QueueUrl → meta (throttle config, incarnation)
  Catalog-->>Server: Queue meta (exists / not found)
  alt queue exists
    Server->>Bucket: charge(queue, action, count, incarnation)
    alt tokens available
      Bucket-->>Server: allowed
      Server->>Handler: Dispatch to queue handler
      Handler-->>Server: Handler result
      Server-->>Client: 200 OK / response
    else throttled
      Bucket-->>Server: denied + retryAfter
      Server-->>Client: 400 Throttling + Retry-After header
    end
  else queue missing
    Server-->>Client: QueueDoesNotExist error
  end

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Pushed addressing all four review findings (Codex + Gemini converged):

3.D split-queue FIFO doc:

• Keyspace backward-compat (CodexP1 + GeminiHigh): §3.1 rewritten — legacy queues use today's keys byte-identically, partitioned queues use a new !sqs|msg|data|p|... prefix with an explicit p| discriminator that cannot collide with legacy keys.
• ReceiveMessage false-empty (CodexP1 + GeminiMedium): §4.2 rewritten — proxies non-local-leader partition scans via the existing leader-proxy (extended to take a partition argument) so a consumer pinned to one endpoint still sees every partition's messages. Added partitionOrder rotation to avoid head-of-line bias.

3.C per-queue throttling doc:

• Hot-path lock contention (GeminiMedium): §3.1 switched to sync.Map lookup + per-bucket sync.Mutex for charge/refill. Cross-queue traffic never serialises on the same lock. Three-step charge operation documented explicitly.
• Metric type (CodexP2): sqs_throttle_tokens_remaining is now correctly a gauge (token budgets go up and down). sqs_throttled_requests_total stays a counter.

Docs-only changes; no implementation gate. +64/-22 lines across the two files.

@claude review

Claude finished @bootjp's task in 3m 1s —— View job

Review

• Read updated design documents
• Verify fix: keyspace backward-compat (3.D)
• Verify fix: ReceiveMessage false-empty (3.D)
• Verify fix: hot-path lock contention (3.C)
• Verify fix: metric type correction (3.C)
• Overall assessment

All four issues from the Codex/Gemini review have been correctly addressed. New issues found below, one of them medium-severity.

All four previous issues: resolved ✓

3.D — Keyspace backward-compat (Gemini high + Codex P1) ✓
The p| discriminator approach cleanly separates the new partitioned keyspace from legacy keys. Legacy paths stay byte-identical; partitioned queues get !sqs|msg|data|p|<queue>|<partition>|…. The reaper enumerating both prefixes is a good forward-compatibility detail.

3.D — ReceiveMessage false-empty (Gemini medium + Codex P1) ✓
§4.2 now proxies non-local-leader partitions via the existing proxyToLeader machinery (extended with a partition argument) and the partitionOrder rotation seeded by RequestId is a nice head-of-line bias fix. The X-Elastickv-Receive-Partition: <k> header approach is clean and consistent with the existing proxy pattern.

3.C — Hot-path lock contention (Gemini medium) ✓
sync.Map for the bucket registry (lock-free reads) + per-bucket sync.Mutex for charge/refill is the right pattern. The three-step charge operation is well-documented. The race-on-insert via LoadOrStore is handled correctly (both racers agree on config).

3.C — Metric type (Codex P2) ✓
sqs_throttle_tokens_remaining is correctly a gauge in both §4.1 (files-touched table) and §7 (operational section). Both sites updated.

New issues in the updated doc

[Medium] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §8.1 — stale failure mode description

The fix to §4.2 eliminated the false-empty problem server-side (proxy fanout handles it), but §8.1 still says:

"Consumer never sees a partition's leader: a consumer that always lands on node A sees only the partitions whose leader is A … Mitigation: document that clients should round-robin endpoints."

This directly contradicts §4.2 — the "round-robin endpoints" mitigation is no longer correct or necessary for this failure mode, since the proxy now handles it transparently. An implementor reading §8.1 in isolation would build the wrong mental model and might even suppress the proxy logic thinking clients are expected to handle distribution themselves.

The failure mode to document now is the performance cost, not the correctness risk: proxy latency under spread deployments (one extra RTT per non-local partition), and the trade-off that a consumer asking for 1 message touches at most 1 remote leader while an "is queue empty?" poll costs N proxy round-trips. Fix this →

[Low] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §3.1 — sqsMsgDataKey variadic signature is a footgun

The proposed constructor:

func sqsMsgDataKey(queueName string, gen uint64, messageID string, partitions ...uint32) []byte {

Using a variadic for the partition discriminator has two issues as implementation guidance:

1. sqsMsgDataKey(q, gen, id, p0, p1) silently ignores p1 — a caller could accidentally pass the wrong count and the compiler won't catch it.
2. Variadic calls allocate even for the zero-argument case in some Go versions, which matters on the legacy hot path.

The doc could instead name two distinct helpers (legacyMsgDataKey / partitionedMsgDataKey) with the thin dispatcher staying internal. The variadic form is fine as a unified constructor for now but the implementation PR should be aware of these edge cases.

[Low] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §3.1 — p| discriminator safety constraint is implicit

The doc correctly states the discriminator prevents collision, but doesn't state why it's safe: valid SQS queue names are alphanumeric + hyphen + underscore and cannot contain |, so no queue name can begin with p|, making the prefix unambiguous. One sentence in §3.1 would nail this closed explicitly — the implementation PR's name validator needs to enforce the | prohibition on queue names (which is standard SQS behaviour, but worth calling out as load-bearing here).

[Low] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §8.5 — upgrade gate mechanism underspecified

"The catalog rejects CreateQueue with PartitionCount > 1 until every node in the cluster reports the new feature flag" is the gate-of-no-return safety mechanism, but the doc doesn't describe how a node reports its feature flag or how the catalog queries all live nodes. The cross-reference to "§3.3.2 admin-forwarding upgrade gate from the admin dashboard design" is opaque without a link. Since this is the safety rail against accidentally creating partitioned queues in a mixed-version cluster, the mechanism deserves a sentence or two (e.g. "each node's health endpoint advertises a capabilities set; the catalog reads all registered nodes from the distribution layer's node registry and gates on all of them reporting HTFIFO").

[Low] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §3.4 — Retry-After: 1 lies for slow-refill queues

The doc justifies the constant Retry-After: 1 as "at the configured refill rate, one second is enough for at least one fresh token" — but this is only true when refillRate >= 1 token/s. A queue configured with SendRefillPerSecond = 0.1 needs 10 seconds for the next token; returning Retry-After: 1 causes 9 unnecessary 400 retries before the client finally gets through. The §3.4 table comment should note this edge case and suggest max(1, ceil(1/refillRate)) seconds, or cap the minimum SendRefillPerSecond at 1 to keep the constant valid.

Summary

The four original issues are cleanly resolved. The §8.1 stale failure mode is the only finding I'd call blocking — it directly contradicts §4.2 and gives implementors the wrong mental model. The remaining three are low-severity clarifications that would improve the implementation PRs' accuracy.
| Branch

Pushed addressing all five findings from your follow-up review (1 medium, 4 low):

3.D split-queue FIFO doc:

• (medium) §8.1 stale failure-mode description rewritten — was contradicting §4.2's proxy fix. Now describes the actual remaining failure (proxy RTT cost under spread deployment) instead of the eliminated false-empty risk.
• (low) §3.1 variadic sqsMsgDataKey replaced with two named constructors (legacyMsgDataKey / partitionedMsgDataKey) + explicit call-site dispatch — no silent argument loss.
• (low) §3.1 p| discriminator safety made explicit: queue-name validator's | prohibition + base32-encoded segments never starting with literal p together guarantee no collision.
• (low) §8.5 upgrade-gate mechanism now concrete: /sqs_health advertises capabilities: ["htfifo"]; catalog polls every registered node, fail-closed on timeout. References PR admin: AdminForward follower-side client + handler integration (P1, partial) #644 as prior art.

3.C per-queue throttling doc:

• (low) §3.4 Retry-After now computed per-queue from ceil((1 - currentTokens) / refillRate) (floored at 1 s for HTTP integer-second granularity) instead of the constant 1. Slow-refill queues no longer cause busy-loops.

Docs-only changes; no implementation gate. +24/-13 lines net across the two files.

@claude review

Claude finished @bootjp's task in 5m 50s —— View job

Review

• Read updated design documents
• Verify fix: §8.1 stale failure-mode description (3.D) — medium
• Verify fix: §3.1 named constructors replacing variadic (3.D) — low
• Verify fix: §3.1 p| discriminator safety explicit (3.D) — low
• Verify fix: §8.5 upgrade-gate mechanism concrete (3.D) — low
• Verify fix: §3.4 Retry-After computed from refill rate (3.C) — low
• Check for new issues
• Overall assessment

All five issues from the last Claude review are correctly resolved. One pre-existing Codex P2 finding from the second-pass review was not in the trigger's scope but remains open; surfacing it here. Two low-severity findings are new.

All five previous issues: resolved ✓

3.D — §8.1 stale failure-mode description (medium) ✓
The failure mode now reads "Proxy RTT under spread deployment" with the explicit note that §4.2 proxies them server-side so false-empty is gone. The N-proxy-round-trip pathological case (asking "is anything here?" against an empty queue) and the long-poll mitigation are both called out. No contradiction with §4.2.

3.D — §3.1 variadic sqsMsgDataKey (low) ✓
Two distinct named constructors: legacyMsgDataKey(queueName, gen, messageID) and partitionedMsgDataKey(queueName, partition, gen, messageID). Call-site dispatch on meta.PartitionCount > 1. No variadic, no silent argument loss.

3.D — §3.1 p| discriminator safety (low) ✓
The argument is now explicit: (1) queue names are restricted to [A-Za-z0-9_-] so | can never appear; (2) the generation field is base32-encoded so it uses only uppercase A-Z and digits 2–7, meaning base32 cannot produce |, which prevents any legacy-key prefix scan from matching a new partitioned-key scan prefix; (3) a note that the implementation PR's name validator must continue to reject |. The argument is present and ultimately correct.

3.D — §8.5 upgrade-gate mechanism (low) ✓
Concrete now: /sqs_health advertises capabilities: ["htfifo"]; the catalog's CreateQueue handler polls every registered node from the distribution layer's node registry (same registry as proxyToLeader); fail-closed on timeout; explicit prior art reference to PR #644's AdminForward gate.

3.C — §3.4 Retry-After formula (low) ✓
Formula is ceil((1.0 - currentTokens) / refillRate) with max(1, ...) floor, inline example: refillRate=0.1 → Retry-After: 10. The "validator keeps refillRate > 0" divide-by-zero note is a good touch.

New findings

[Medium] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §3.3 — FifoThroughputLimit=perQueue not implemented in partitionFor

This was flagged as Codex P2 in the second-pass review but was not in the trigger's scope (trigger addressed Claude's five findings only). It remains open in the doc.

§3.2 documents FifoThroughputLimit:

"the alternative 'perQueue' reduces the partition assignment to a single partition (effectively N=1)"

But §3.3's partitionFor pseudocode only gates on meta.PartitionCount <= 1:

if meta.PartitionCount <= 1 {
    return 0
}
// falls through to hash for any PartitionCount > 1

A queue created with PartitionCount=8 + FifoThroughputLimit=perQueue would hash across all 8 partitions, directly contradicting §3.2's documented semantics. The fix is one line: Fix this →

// Fixed guard:
if meta.PartitionCount <= 1 || meta.FifoThroughputLimit == "perQueue" {
    return 0
}

[Low] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §4.1 — pseudocode still uses the old sqsMsgDataKey name

§3.1 renamed the constructor, but §4.1 step 5 still references the old form:

5. Build the OCC OperationGroup with partition-aware keys:
     dataKey  = sqsMsgDataKey(queue, partitionIndex, gen, msgID)

An implementor reading §4.1 in isolation gets conflicting guidance vs. §3.1. The pseudocode should dispatch via the named constructors introduced in §3.1: Fix this →

[Low] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §3.4 — Retry-After formula hardcodes 1.0 regardless of batch size

The fix correctly computes Retry-After from refillRate. But the formula numerator is always 1.0:

secondsToNextToken := math.Ceil((1.0 - currentTokens) / refillRate)

For a SendMessageBatch of 10 entries (charge = 10) against a bucket with 0 tokens and refillRate = 1.0, this gives Retry-After: 1 — but 10 seconds are needed for the batch to succeed. The client burns 9 retries before accumulating enough tokens. The formula should use the requested count instead of the constant 1.0:

secondsToNextToken := math.Ceil((float64(requestedCount) - currentTokens) / refillRate)

This matters most for large batches with slow-to-medium refill rates; for fast refill or single operations the constant-1 floor masks the gap anyway.

Summary

All five issues from the last review are cleanly fixed. The FifoThroughputLimit=perQueue routing gap (Codex P2 from the second-pass review, not previously in Claude's scope) is the only finding here I'd call blocking before the implementation PRs — it means the routing logic in §3.3 contradicts the semantics documented in §3.2. The other two are low-severity doc cleanups.
| Branch

Pushed addressing all three findings from your third review (1 medium, 2 low):

3.D split-queue FIFO doc:

• (medium) §3.3 partitionFor now also short-circuits when FifoThroughputLimit == "perQueue" — fixes the contradiction with §3.2's documented semantics. Codex P2 from the second-pass review, now resolved.
• (low) §4.1 step 5 pseudocode now dispatches through the §3.1 named constructors (legacyMsgDataKey / partitionedMsgDataKey) instead of the old variadic form. No more conflicting guidance between §3.1 and §4.1.

3.C per-queue throttling doc:

• (low) §3.4 Retry-After formula now uses requestedCount instead of the hardcoded 1.0 numerator. SendMessageBatch of 10 against refillRate=1.0 correctly returns Retry-After: 10 instead of 1.

Docs-only changes; +18/-7 lines net.

@claude review

Claude finished @bootjp's task in 2m 9s —— View job

Review

• Read updated design documents
• Verify fix: §3.3 partitionFor FifoThroughputLimit check (medium)
• Verify fix: §4.1 step 5 named constructor dispatch (low)
• Verify fix: §3.4 Retry-After uses requestedCount (low)
• Check for new issues (including open Codex P1 findings)
• Overall assessment

All three issues from the trigger are correctly resolved. Two Codex P1 findings from the previous review remain open and are surfaced below (they were not in scope of this push). Two new low-severity issues found.

All three previous issues: resolved ✓

3.D — §3.3 FifoThroughputLimit=perQueue routing (medium) ✓
The guard is now:

if meta.PartitionCount <= 1 || meta.FifoThroughputLimit == "perQueue" {
    return 0
}

The comment explicitly documents that perQueue collapses all groups to partition 0 regardless of PartitionCount, and calls out the Codex P2 catch. The contradiction with §3.2 is resolved.

3.D — §4.1 step 5 named constructor dispatch (low) ✓
Step 5 now uses the explicit if meta.PartitionCount > 1 dispatch branching to partitionedMsgDataKey / legacyMsgDataKey (and analogues for visKey / groupKey). No more reference to the old variadic sqsMsgDataKey. §3.1 and §4.1 are now consistent.

3.C — §3.4 Retry-After uses requestedCount (low) ✓
Formula is now:

needed              := float64(requestedCount) - currentTokens
secondsToNextRefill := math.Ceil(needed / refillRate)
retryAfter          := max(1, int(secondsToNextRefill))

Both cases are correctly handled: a SendMessageBatch of 10 against refillRate=1.0 gives Retry-After: 10; a single SendMessage against refillRate=0.1 also gives Retry-After: 10. The floor-1 and divide-by-zero notes are preserved.

Open Codex P1 findings from the previous round (not addressed in this push)

These were raised against f289d6c291 and are still open in the current doc.

[Medium] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §3.1 — bucket cache not invalidated on SetQueueAttributes

§3.1 describes LoadOrStore on first use: "build the bucket from queue meta and LoadOrStore it." Once a bucket is stored, the doc describes no path for invalidating or refreshing it when SetQueueAttributes changes the throttle config. A queue created with SendRefillPerSecond = 10, then lowered to 1 via SetQueueAttributes, continues to allow 10 RPS until the idle-eviction sweep removes the old bucket entry (default 1h window). An operator who lowers limits to stop a noisy tenant would see the effect delayed by up to an hour.

The fix is a sentence in §3.1: SetQueueAttributes must call buckets.Delete(key) for all action-keys of the updated queue after committing the meta change to Raft, so the next request re-initialises the bucket from the new config. Fix this →

[Medium] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §5 — --raftSqsMap reuse conflicts with endpoint-address semantics

§5 proposes extending --raftSqsMap with a new queueName:N=group1,...,groupN syntax. The existing flag maps raftAddr=sqsAddr (endpoint routing for proxyToLeader). Reusing the same flag for a completely different concern — partition-to-Raft-group assignment — makes the config surface ambiguous: a single flag entry must now be parsed as either an address mapping or a partition assignment, and the proxyToLeader fanout in §4.2 depends on correctly resolving the per-partition leader's SQS endpoint from this same flag. Mixing the two intents in one parsing path risks a subtle bug where a partitioned-queue config entry is accidentally interpreted as a raftAddr=sqsAddr pair (or vice versa), silently producing the wrong proxy target.

A new flag (e.g. --raftSqsFifoMap) or a clearly separated config section avoids the parsing collision and lets the two concerns evolve independently. Fix this →

New findings

[Low] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §9 — no test case for FifoThroughputLimit=perQueue

§9's unit test list covers PartitionCount <= 1 and empty MessageGroupId fast-paths but does not include a test that verifies FifoThroughputLimit=perQueue routes to partition 0 even when PartitionCount = 8. The perQueue short-circuit is the fix this push added to §3.3 — without an explicit test the correctness is trivially regressionable. One test case: partitionFor({PartitionCount: 8, FifoThroughputLimit: "perQueue"}, "any-group-id") == 0. Fix this →

[Low] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §4.3 — "sharded by MessageGroupId" is the wrong key

§4.3 describes the forward-compatibility path for Phase 3.D:

"each partition gets its own bucket, sharded by MessageGroupId"

The bucket key should be (queueName, partitionID), not (queueName, MessageGroupId). MessageGroupId is the input to partitionFor — the partition index it maps to is what identifies which leader owns the bucket. Using MessageGroupId as the key would create one bucket per group value (unbounded, attacker-amplifiable map size); using partitionID creates exactly PartitionCount buckets per queue (bounded by design). The §4.3 sentence should read "each partition gets its own bucket, keyed by (queueName, partitionID)." Fix this →

Summary

All three fixes from this push are cleanly applied. The two Codex P1 findings from the previous review — bucket cache invalidation on SetQueueAttributes (3.C) and --raftSqsMap parsing ambiguity (3.D) — remain the only items I'd consider blocking before implementation PRs. Both are design-level gaps that implementation code will have to resolve anyway; better to get the doc right first. The two new findings are low-severity doc corrections.
| Branch

Pushed addressing all four findings from your fourth review (2 medium, 2 low):

3.C per-queue throttling doc:

• (medium) §3.1 now requires SetQueueAttributes to call buckets.Delete(key) after the Raft commit so a config change takes effect on the next request, not after the 1h idle-eviction window. Race with LoadOrStore is benign (rebuilt bucket starts at full capacity, same as failover).
• (low) §4.3 forward-compat fixed: bucket key for split-FIFO is (queueName, partitionID), not MessageGroupId — the latter would create unbounded buckets per group value.

3.D split-queue FIFO doc:

• (medium) §5 now uses a new dedicated --sqsFifoPartitionMap flag for partition-to-Raft-group assignment instead of overloading --raftSqsMap (which keeps doing endpoint mapping for proxyToLeader). Eliminates the parsing ambiguity. §11 rollout table updated.
• (low) §9 unit test list now includes FifoThroughputLimit=perQueue with PartitionCount=8 routes every group to partition 0 to lock down the §3.3 short-circuit guard against regression.

Docs-only changes; +9/-6 lines net.

@claude review

Claude finished @bootjp's task in 3m 26s —— View job

Review

• Read updated design documents
• Verify fix: §3.1 SetQueueAttributes bucket invalidation (3.C) — medium
• Verify fix: §4.3 bucket key (queueName, partitionID) vs MessageGroupId (3.C) — low
• Verify fix: §5 --sqsFifoPartitionMap dedicated flag (3.D) — medium
• Verify fix: §9 FifoThroughputLimit=perQueue test case (3.D) — low
• Check for remaining open Codex P1 items from prior rounds
• Check for new issues
• Overall assessment

All four issues from the trigger are correctly resolved. Two Codex P1 findings first surfaced in my fourth-round review — and noted there as "not in scope of this push" — remain open in the current doc. One new low-severity finding.

All four trigger fixes: resolved ✓

3.C — §3.1 SetQueueAttributes bucket cache invalidation (medium) ✓
The paragraph is explicit and accurate: after the Raft commit, the handler calls buckets.Delete(key) for all three action-keys (Send, Recv, Default). The race with LoadOrStore is correctly noted as benign (fresh bucket starts at full capacity, same as failover). The 1h idle-eviction context makes the urgency of the invalidation clear.

3.C — §4.3 bucket key (queueName, partitionID) (low) ✓
The corrected key is explicit, the MessageGroupId-as-key footgun is called out with the reason (unbounded attacker-amplifiable map), and the bucketKey struct is noted to gain a partition uint32 field when Phase 3.D lands. Consistent with §3.1's struct definition.

3.D — §5 --sqsFifoPartitionMap dedicated flag (medium) ✓
The new flag is introduced with clear syntax (orders.fifo:8=group-7,...group-14), the separation from --raftSqsMap's endpoint-mapping role is stated explicitly and the reason (parsing ambiguity → wrong proxy target) is documented. The rollout table (§11, PR 4) also references the new flag by name.

3.D — §9 FifoThroughputLimit=perQueue test case (low) ✓
The bullet is present, includes the concrete call form (FifoThroughputLimit = "perQueue" with PartitionCount = 8 → partition 0 for every group ID), and the regression rationale is spelled out ("the perQueue branch is a one-line guard that could easily be dropped during a refactor"). Good.

Still-open Codex P1 findings from d424d18d7c (surfaced in my last review, not addressed in this push)

These were called out as "not in scope of this push" in my fourth-round review summary but were not deferred to a follow-up issue — they are design-level gaps the implementation PRs will have to resolve.

[Medium] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §3.1 — DeleteQueue does not invalidate the bucket cache

§3.1 now correctly invalidates buckets on SetQueueAttributes, but says nothing about DeleteQueue. A queue deleted and recreated with the same name inherits the old in-memory bucket's token balance and — if the new queue was created without throttle config — the old bucket's capacity/refillRate as well, until the idle-eviction sweep (1h default). An operator who uses DeleteQueue+CreateQueue to reset a noisy queue's state gets a surprise: the throttle limits from the deleted queue follow the new one for up to an hour.

The fix is one sentence: "DeleteQueue calls buckets.Delete for all action-keys of the queue (same as SetQueueAttributes), so a recreated same-name queue starts with a clean bucket." Alternatively, include generation in bucketKey so old and new incarnations never share a bucket entry. Fix this →

[Medium] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §4.2 — WaitTimeSeconds not bounded across the partition fanout

The fanout iterates partitions sequentially until MaxNumberOfMessages are collected. If each proxied sub-call independently long-polls for WaitTimeSeconds, an empty partitioned queue with WaitTimeSeconds = 20 and PartitionCount = 8 holds the connection for up to 160 s before returning empty — well past any reasonable client or load-balancer timeout. The doc notes "FIFO consumers are expected to use long-poll (WaitTimeSeconds)" as a mitigant for the N-round-trip cost, which inadvertently encourages the very configuration that creates the problem.

§4.2 should specify how the wait budget is managed: either (a) a single top-level deadline is threaded through as remaining_wait = WaitTimeSeconds - elapsed_so_far and passed to each sub-call, so the total wait is bounded by WaitTimeSeconds; or (b) partitions are probed in parallel and the first non-empty result short-circuits. Option (a) matches how AWS's internal partition fanout works; option (b) is simpler to implement but changes the per-partition polling semantics. Either choice should be explicit. Fix this →

New finding

[Low] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §3.4 — reference to --raftSqsMap for partition routing contradicts §5

§3.4 states: "The existing kv/shard_router.go plus the multi-shard configuration flags (--raftSqsMap) already support this — each (queueName, partition) pair becomes its own routing key."

--raftSqsMap maps raftAddr=sqsAddr (endpoint pairs for proxyToLeader); it does not map (queueName, partition) → Raft group. §5 is explicit that partition-to-Raft-group assignment requires the new --sqsFifoPartitionMap flag and that --raftSqsMap is unchanged by this design. An implementor reading §3.4 would conclude partition routing is free with the existing flag, reach §5, and be confused about what needs to be added. The §3.4 sentence should say that while the router infrastructure (kv/shard_router.go) supports multi-group routing today, the partition-to-group assignment configuration requires the new --sqsFifoPartitionMap flag described in §5. Fix this →

Summary

All four trigger fixes are cleanly applied. The two Codex P1 items that have been open since d424d18d7c — DeleteQueue bucket lifecycle and WaitTimeSeconds budget management across the fanout — are the only findings I'd call blocking before implementation PRs start. Both are design decisions the implementation code has to embody either way; resolving them in the doc now prevents the PR author from having to reverse a committed approach. The §3.4/§5 inconsistency is low-severity cleanup.
| Branch

Pushed addressing all three findings from your fifth-round review (2 medium, 1 low):

3.C per-queue throttling — §3.1 DeleteQueue bucket lifecycle (medium):

• New paragraph immediately after the SetQueueAttributes invalidation block. DeleteQueue now also calls buckets.Delete(key) for every bucketKey of the deleted queue (Send, Recv, Default), so a DeleteQueue+CreateQueue with the same name no longer inherits the previous incarnation's token balance/limits for up to 1h. Documented the generation-in-bucketKey alternative and the reason for picking the explicit Delete (cheaper, mirrors the SetQueueAttributes pattern).

3.D split-queue FIFO — §4.2 WaitTimeSeconds budget across fanout (medium):

• Pseudocode reworked: step 3 sets deadline := start + WaitTimeSeconds (cap 20s); step 4a/4c/4d compute remainingWait := max(0, deadline - now()) and pass it as the per-sub-call long-poll budget. Total wall-clock wait is now bounded by the original WaitTimeSeconds regardless of PartitionCount. Without this, an empty queue with WaitTimeSeconds=20 + PartitionCount=8 would hold the connection for up to 160s.
• Added a "Why the shared deadline matters" paragraph documenting the bug class and the rejected alternative (parallel probe with shared cancellation — rejected because it erases the partitionOrder rotation).
• §8.1 dropped the contradictory "long-poll mitigates the RTT cost" sentence; long-poll was actually the trigger for the bug.

3.D split-queue FIFO — §3.4 --raftSqsMap reference (low):

• Rephrased to distinguish the router infrastructure (kv/shard_router.go, which already supports multi-group routing by key) from the configuration surface (--sqsFifoPartitionMap, the new flag from §5). Calls out explicitly that conflating the two would parse partition assignments as endpoint pairs and route to the wrong leader. Aligns with §5.

@claude review

Claude finished @bootjp's task in 3m 20s —— View job

Review

• Read updated design documents
• Verify fix: §3.1 DeleteQueue bucket invalidation (3.C) — medium
• Verify fix: §4.2 WaitTimeSeconds shared deadline across fanout (3.D) — medium
• Verify fix: §3.4 --raftSqsMap reference cleanup (3.D) — low
• Check open Codex P1 items from last round
• Check for new issues
• Overall assessment

All three fixes from the trigger are correctly applied. Two Codex P1 items raised in the sixth-round review remain open and are the only findings I would consider blocking before implementation PRs. One new low-severity finding.

All three trigger fixes: resolved ✓

3.C — §3.1 DeleteQueue bucket lifecycle (medium) ✓
The paragraph is explicit and accurate: after the Raft commit that purges the queue meta, buckets.Delete(key) is called for all three action-keys. The race with LoadOrStore is correctly noted as benign. The alternative (generation-in-bucketKey) is documented and the reason for rejecting it is clear. The 1 h idle-eviction context makes the urgency obvious.

3.D — §4.2 WaitTimeSeconds shared deadline (medium) ✓
Step 3 sets deadline := start + WaitTimeSeconds (capped at 20s); step 4a computes remainingWait := max(0, deadline - now()) and passes it to each sub-call. The "Why the shared deadline matters" paragraph correctly explains the 160 s pathological case that the fix eliminates, and the rejected parallel-probe alternative is documented with the reason (erases partitionOrder rotation semantics). §8.1 no longer contains the contradictory long-poll mitigation sentence.

3.D — §3.4 --raftSqsMap reference cleanup (low) ✓
The distinction is now explicit: kv/shard_router.go already supports multi-group routing (no changes needed there), but the configuration surface for partition-to-group assignment is --sqsFifoPartitionMap (§5), not --raftSqsMap. The consequence of conflating the two ("parse partition assignments as endpoint pairs and route to the wrong leader") is called out directly.

Still-open Codex P1 findings from f6bd33ab70 (sixth-round Codex review)

These were raised in the commit-by-commit Codex review on PR #664 and have not yet been addressed.

[Medium] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §3.2 — validator does not bound capacity to max single-request charge

The validator enforces capacity >= refill but not capacity >= max_single_request_charge. The maximum single-request charge from the Send bucket is 10 (a full SendMessageBatch). A queue configured with SendCapacity = 5 creates a state where every SendMessageBatch of > 5 entries is permanently unserviceable — the bucket can only hold 5 tokens and can never accumulate 10. The client receives Throttling forever, Retry-After grows proportionally, and there is no way to recover without reconfiguring the queue. The Retry-After formula (§3.4) correctly computes ceil((10 - 5) / refillRate) but that only tells the client "wait N seconds" before they'll be throttled again.

The fix is one validation rule: each capacity that covers a batch verb must be at least the maximum charge of any single request through that bucket. For Send: SendCapacity >= 10. For Recv: RecvCapacity >= 10 (DeleteMessageBatch is also up to 10). Document the constraint in §3.2 and reject with InvalidAttributeValue at SetQueueAttributes. Fix this →

[Medium] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §8 item 5 — no runtime safeguard against a downgraded node becoming leader after HT-FIFO queue creation

§8 item 5 says "the catalog rejects CreateQueue with PartitionCount > 1 until every node reports htfifo" — a create-time gate. But the same item acknowledges "older nodes that try to scan a partitioned queue's keyspace will simply not find anything." There is no runtime protection against the following sequence:

1. All nodes have htfifo; a partitioned queue orders.fifo is created.
2. Node A is upgraded back (or replaced by a rollback image) and rejoins without htfifo.
3. Node A is elected leader for the partition-3 Raft group.
4. Node A's ReceiveMessage scans the old prefix (!sqs|msg|data|orders.fifo|...) and finds nothing — false-empty reads.
5. Node A's SendMessage writes the old key format — data on partition 3 is written to the wrong prefix and the reaper won't find it under the new prefix.

The create-time gate does not cover this; it only prevents new partitioned queues from being created in a mixed-version cluster. Once a partitioned queue exists, any node that serves it must also understand the new prefix.

The proposal needs a runtime safeguard: a node that does not have htfifo should refuse to become leader (or refuse to serve the SQS handler) for a Raft group that contains any queue with PartitionCount > 1. A practical implementation: on startup (or on leadership acquisition), query the catalog for partitioned queues in the local shard; if any exist and the binary lacks htfifo, refuse leadership with an explicit log message. This is the node-admission variant of the create-time gate. Fix this →

New finding

[Low] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §3.1 — bucketKey struct definition and DeleteQueue paragraph are inconsistent on whether partitionID is in the current key

The struct definition in §3.1 is:

type bucketKey struct {
    queue  string
    action string  // "Send" | "Receive" | "*"
}

But the DeleteQueue paragraph (added in this latest push) says:

"so we keep bucketKey = (queueName, partitionID, action) for now"

partitionID is not in the current struct. §4.3 explicitly says the struct "grows a partition uint32 field" when Phase 3.D lands. The DeleteQueue paragraph should say (queueName, action) — partitionID is a future addition. An implementor reading §3.1 end-to-end will see the struct definition conflict with the inline text, and if they include partitionID in Phase 3.C based on the paragraph, the §4.3 forward-compat note will be confusing. Fix this →

Summary

All three trigger fixes are cleanly applied. The two Codex P1 items from the sixth-round review — bucket capacity bound on max batch charge (3.C §3.2) and runtime safeguard for downgraded nodes post-HT-FIFO creation (3.D §8) — are the only findings I'd call blocking before the implementation PRs. Both represent design gaps that will have to be resolved in code regardless; better to settle the spec now. The bucketKey inconsistency is a low-severity cleanup.
| Branch

Pushed addressing all three findings from your sixth-round review (2 medium Codex P1 + 1 low Claude):

3.C per-queue throttling — §3.2 capacity ≥ max batch charge (Codex P1, medium):

• New fourth bullet on the SetQueueAttributes validator: when SendCapacity > 0 it must also be ≥ 10, and likewise RecvCapacity > 0 → ≥ 10. Justified inline against the §3.3 charging table (SendMessageBatch / DeleteMessageBatch charge up to 10 each). Documented the failure mode the rule prevents (permanently unserviceable batches with no recovery short of re-running SetQueueAttributes) and the rejection envelope (InvalidAttributeValue with the per-bucket minimum named so the operator sees the cause). Default* is exempt because no batch verb is in scope of the catch-all bucket today; explicit forward note for when one moves there.

3.D split-queue FIFO — §8 item 5 runtime safeguard for downgraded leaders (Codex P1, medium):

• New paragraph after the capability advertisement mechanism. Describes the failure sequence the create-time gate misses (downgrade after a partitioned queue exists → false-empty reads + key-prefix corruption from the rolled-back leader). Specifies the safeguard: on startup and on leadership acquisition for an SQS Raft group, enumerate local-shard queues with PartitionCount > 1; if any exist and the binary lacks htfifo, refuse leadership with an explicit log line and TransferLeadership away (group becomes leaderless if no peer is willing — desired fail-closed behaviour). Hooks into the existing leadership-acquisition path in kv/lease_state.go so steady-state cost is zero. Explicitly required before the §11 PR 4 rollout step that marks the binary htfifo-eligible.

3.C per-queue throttling — §3.1 bucketKey text/struct consistency (Claude low):

• The DeleteQueue paragraph said "we keep bucketKey = (queueName, partitionID, action) for now" but the struct definition above has only queue + action (no partitionID until Phase 3.D per §4.3). Corrected to "for Phase 3.C we keep bucketKey = (queueName, action) per the struct definition above — the partition field is added later by Phase 3.D, see §4.3".

@claude review

Claude finished @bootjp's task in 3m 10s —— View job

Review

• Read updated design documents
• Verify fix: §3.2 capacity ≥ max batch charge (3.C) — Codex P1 medium
• Verify fix: §8 item 5 runtime safeguard for downgraded leaders (3.D) — Codex P1 medium
• Verify fix: §3.1 bucketKey text/struct consistency (3.C) — Claude low
• Check for new issues (including open Codex P1 from last round)
• Overall assessment

All three fixes from this push are correctly applied. Two Codex P1 findings from the previous round's Codex review (on commit 624d411a93) were not in scope of this push and remain open. One new low-severity finding.

All three trigger fixes: resolved ✓

3.C — §3.2 capacity ≥ max batch charge (Codex P1, medium) ✓
The fourth validation bullet is precise: SendCapacity > 0 → SendCapacity ≥ 10, RecvCapacity > 0 → RecvCapacity ≥ 10, justified by the §3.3 charging table. The failure mode (permanent unserviceability for full batches) is documented inline, the rejection envelope (InvalidAttributeValue) is named, and the Default* exemption is correctly scoped with an explicit forward note. The Codex P1 attribution is clear. Clean.

3.D — §8 item 5 runtime safeguard for downgraded leaders (Codex P1, medium) ✓
The new paragraph accurately describes the failure sequence the create-time gate misses (downgrade after partitioned queue exists → false-empty reads + key-prefix corruption). The safeguard is concrete: catalog enumeration on startup + leadership acquisition, TransferLeadership on capability mismatch, fail-closed if no willing peer. The hook into kv/lease_state.go, the zero steady-state cost claim, and the ordering constraint relative to §11 PR 4 are all present and correct.

3.C — §3.1 bucketKey text/struct consistency (Claude low) ✓
The DeleteQueue paragraph now reads "for Phase 3.C we keep bucketKey = (queueName, action) per the struct definition above — the partition field is added later by Phase 3.D, see §4.3." Consistent with the struct definition on lines 92–95 and the forward-compat note in §4.3. The previous partitionID mention is gone.

Still-open Codex P1 findings from the commit 624d411a93 Codex review

These were new findings added by Codex on the last commit round; they were not in scope of this push.

[Medium] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §3.1 — action key names in cache invalidation don't match struct definition

The bucketKey.action field is defined at line 94 as "Send" | "Receive" | "*". But both the SetQueueAttributes and DeleteQueue invalidation paragraphs (lines 114 and 116) delete keys for "Send", "Recv", "Default":

calls buckets.Delete(key) for every bucketKey belonging to the updated queue (Send, Recv, Default)

"Recv" ≠ "Receive" and "Default" ≠ "*". An implementation that follows the struct definition will never match the keys the invalidation code tries to delete, so Receive and Default buckets will remain stale after a config update — defeating the correctness guarantee the SetQueueAttributes invalidation paragraph is trying to establish. The fix is one-word changes in both paragraphs: use "Receive" and "*" (matching the struct), or update the struct comment to use "Recv" / "Default" and define those as the canonical values. Fix this →

[Medium] docs/design/2026_04_26_proposed_sqs_split_queue_fifo.md §3.2 — FifoThroughputLimit and DeduplicationScope are mutable after first send

Line 106 states only PartitionCount is immutable after first SendMessage. But routing in partitionFor also depends on FifoThroughputLimit — the perQueue short-circuit (added in an earlier round) collapses all groups to partition 0 regardless of PartitionCount. If an operator flips FifoThroughputLimit from perMessageGroupId to perQueue after messages exist, every MessageGroupId that was previously distributed across N partitions now routes to partition 0, splitting those groups across multiple partitions and violating within-group FIFO ordering for any in-flight messages. Similarly, DeduplicationScope affects how the dedup key is scoped; changing it live would either duplicate or suppress messages unexpectedly.

The fix is extending the immutability sentence: "PartitionCount, FifoThroughputLimit, and DeduplicationScope are all immutable after first SendMessage" — since all three affect routing or dedup semantics in ways that cannot be changed without a full drain. Fix this →

New finding

[Low] docs/design/2026_04_26_proposed_sqs_per_queue_throttling.md §6 — integration test example uses an attribute value that §3.2 now rejects

§6 item 2 reads:

"Configure a queue with SendCapacity=5 SendRefillPerSecond=1. Send 5 messages back-to-back → all 200."

But §3.2's new fourth validation bullet requires SendCapacity ≥ 10 when non-zero. A SetQueueAttributes call with SendCapacity=5 would now return InvalidAttributeValue before the test even creates its first message — meaning the test as written would fail at setup, not at message 6. The example should use SendCapacity=10 (or higher) so the test is consistent with the validator it is supposed to be testing against. Fix this →

Summary

All three trigger fixes are cleanly applied. The two Codex P1 findings from the previous round's review — action key naming mismatch in cache invalidation (3.C §3.1) and FifoThroughputLimit / DeduplicationScope mutability (3.D §3.2) — are the only items I'd consider blocking before implementation PRs. Both are design-level decisions that implementation code would have to make regardless; getting the doc right now prevents the PRs from encoding the wrong choice. The integration test inconsistency is low-severity cleanup.
| Branch

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Pushed addressing all three findings from your seventh-round review (2 medium Codex P1 + 1 low Claude):

3.C per-queue throttling — §3.1 action key naming consistency (Codex P1, medium):

• Both invalidation paragraphs (SetQueueAttributes, DeleteQueue) now read (Send, Receive, *— the canonical action values from thebucketKey struct above). The prior (Send, Recv, Default) was the JSON config field-name vocabulary, not the in-memory map key — implementations following the struct would have left Receive and * buckets stale.
• Added a new "Config-field → bucket-action mapping" paragraph immediately after the sqsQueueThrottle struct definition. Explicitly maps Send* → bucketKey{action:"Send"}, Recv* → bucketKey{action:"Receive"}, Default* → bucketKey{action:"*"}, and pins which vocabulary to use where (config-field for the JSON contract / GetQueueAttributes response; bucket-action for in-memory bucket-store ops). Implementation must apply the mapping after SetQueueAttributes commit.

3.D split-queue FIFO — §3.2 immutability extended to FifoThroughputLimit + DeduplicationScope (Codex P1, medium):

• Sentence rewritten: "PartitionCount, FifoThroughputLimit, and DeduplicationScope are all immutable after first SendMessage." Three bullets explain the failure mode for each:
  • PartitionCount — re-hashing every message, breaks in-flight ordering, multi-second op (existing reasoning).
  • FifoThroughputLimit — flipping perMessageGroupId ↔ perQueue activates/deactivates the §3.3 short-circuit; in-flight messages stay where they are, new sends for the same group route to a different partition, within-group FIFO is silently violated.
  • DeduplicationScope — changing scope reshapes the dedup key ((queue, dedupId) vs (queue, partitionId, dedupId) vs (queue, MessageGroupId, dedupId)); narrowing scope can resurrect previously-deduped duplicates, widening can suppress legitimately-new sends that collide with still-cached prior-scope entries.
• Concluding sentence pins the unifying invariant: each attribute participates in a routing or dedup decision whose correctness depends on every existing message having been written under one consistent value, and live mutation creates incompatible "before"/"after" sets the runtime cannot reconcile without a full drain.

3.C per-queue throttling — §6 item 2 test capacity (Claude low):

• Bumped from SendCapacity=5 SendRefillPerSecond=1 / 5 messages / 6th throttled to SendCapacity=10 SendRefillPerSecond=1 / 10 messages / 11th throttled. Added a parenthetical inline note explaining why the value changed (the new §3.2 validator would have rejected SendCapacity=5 at setup).

@claude review

@claude review

Claude encountered an error —— View job

I'll analyze this and get back to you.

@claude review